-
Notifications
You must be signed in to change notification settings - Fork 359
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix flaky shutdown #2312
Fix flaky shutdown #2312
Conversation
ec7a0f4
to
2ff10fa
Compare
2ff10fa
to
fa9f685
Compare
564ea71
to
ce11d92
Compare
c90fb13
to
6438623
Compare
6438623
to
c783236
Compare
e169238
to
aa15c20
Compare
aa15c20
to
898899e
Compare
@@ -105,25 +116,41 @@ class IsolateDispatcher { | |||
} else { | |||
var future = ReusableIsolate.spawn(_isolateMain); | |||
isolate = await future; | |||
isolate.receivePort.listen((message) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it necessary to pull this logic out here, rather than keeping it in ReusableIsolate
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From previous implementation:
// Parse out the compilation ID and surface the [ProtocolError] as an
// error. This allows the [IsolateDispatcher] to notice that an error
// has occurred and close out the underlying channel.
// Protocol errors have already been through [_handleError] in the child
// isolate, so we just send them as-is and close out the underlying
// channel.
First, the motivation to remove the intermediate StreamController
is to eliminate two unnecessary performance overheads:
- To eliminate the overhead of deserializing
ProtocolError
packet, passing it throughaddError
, and re-serializing it back to packet. - We don't actually care about theProtocolError
object, all we care is that the category 2 indicate the packet contains a ProtocolError. - To eliminate the overhead of allocating new
StreamController
for every single compilation.
In regarding of why the receivePort.listen
is moved to IsolateDispatcher
, IMO logically how the message is sent across Isolates is a contract between IsolateDispatcher
and CompilationDispatcher
, ReusableIsolate
should purely be a passthrough helper just to help with creating and managing isolate for repeated use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're right that putting the decoding logic here makes the most sense. But I'm not a big fan of giving the caller responsibility for maintaining a single stream subscription across multiple borrows, and I'm not very worried about the overhead of an extra StreamController
. Can we make .borrow()
return a subscription that just forwards a view of the Stream<object?>
which closes automatically once .return()
is called?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not precisely a new Stream
each time, but I made .borrow
take a new onData
function each time, which behaves somewhat like the previous implementation but without creating new StreamController
.
7def05e
to
d95b74c
Compare
try { | ||
return _mailbox.take(); | ||
} on StateError catch (_) { | ||
// [_mailbox] has been closed, exit the current isolate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this could still use some detail on why exiting immediately rather than waiting for futures to wind down is valuable. Think of it from the perspective of someone reading this without any context on the flakiness that used to occur.
@@ -105,25 +116,41 @@ class IsolateDispatcher { | |||
} else { | |||
var future = ReusableIsolate.spawn(_isolateMain); | |||
isolate = await future; | |||
isolate.receivePort.listen((message) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're right that putting the decoding logic here makes the most sense. But I'm not a big fan of giving the caller responsibility for maintaining a single stream subscription across multiple borrows, and I'm not very worried about the overhead of an extra StreamController
. Can we make .borrow()
return a subscription that just forwards a view of the Stream<object?>
which closes automatically once .return()
is called?
8952575
to
7e978d3
Compare
Co-authored-by: Natalie Weizenbaum <nweiz@google.com>
950316a
to
ea383c7
Compare
1fc7361
to
eb5afb1
Compare
eb5afb1
to
f8c2ffd
Compare
Thanks for working on this and helping reduce flakiness! |
This PR fixes a few shutdown reliability issues including the following flaky test on GitHub Actions windows runner: